chore(factorio-cycle-calculator): Address the Comments#116
Conversation
hcoona
force-pushed
the
dev/shuaizhang/factorio-calculator
branch
from
2841e43 to
ac70967
Compare
There was a problem hiding this comment.
Pull request overview
Updates the private Factorio cycle calculator app/scripts to incorporate prior review feedback: remove hard-coded local paths, improve robustness when parsing assets/data, and refine solver/productivity handling.
Changes:
- Removed hard-coded local Factorio paths and shifted defaults toward env/user-provided inputs.
- Added machine
base_productivitysupport and included it in per-machine rate calculations. - Improved robustness/UX: safer icon loading, better error handling for invalid recipe chains, and
__main__guarding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/private/app/factorio-cycle-calculator/app.py | Removes hard-coded defaults, adds base productivity to machines, improves icon loading and solver validation, and adds if __name__ == "__main__" guard. |
| src/private/app/factorio-cycle-calculator/.AGENT/scripts/check_icons_and_locale.py | Removes hard-coded defaults for data-raw path, supports env-based defaults, and tightens validation/arg handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/private/app/factorio-cycle-calculator/.AGENT/scripts/check_icons_and_locale.py
Outdated
Show resolved
Hide resolved
💡 Codex Review Apply demand conversion using the selected rate unit
The solver always computes three/src/private/app/factorio-cycle-calculator/app.py Lines 1080 to 1081 in 2841e43 Respect per-result productivity exclusions
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
True positive on both points. Updated demand conversion to divide by the selected unit multiplier (per-min vs per-sec), and preserved per-result productivity exclusions by tracking ignored-by-productivity outputs and skipping the multiplier for those results. |
|
@codex review |
hcoona
force-pushed
the
dev/shuaizhang/factorio-calculator
branch
from
ac70967 to
8481887
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/private/app/factorio-cycle-calculator/app.py:354
- The direct float() conversion is unsafe for dynamically-typed data-raw fields. If base_effect.get("productivity") returns an unexpected type (e.g., dict or list), this will raise TypeError and crash catalog building. Use parse_effect_bonus() instead for consistent type coercion that handles edge cases gracefully.
crafting_speed = float(proto.get("crafting_speed", 1.0))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/private/app/factorio-cycle-calculator/.AGENT/scripts/check_icons_and_locale.py
Show resolved
Hide resolved
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
No description provided.